-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
single line transaction list #1191
base: main
Are you sure you want to change the base?
single line transaction list #1191
Conversation
I like your new format more than the current. But the old multi-line format was there for some reason. I think @j-mracek should be able to explain the reason. |
The transaction table is a tricky thing. It contains operation install, remove with only one version and upgrade that contains in general two versions. But there are other combinations. like upgrading package might replace multiple packages and installing package might also replace multiple packages. Then a combination of the one version showed in version column and others in new lines might be confusing. There is additional requirements
PS: This PR is the best approach how to reduce lines in output, but still I am not sure whether it will be OK for 80 characters terminal. |
…tion' into single_line_transaction
It is not possible to put all this output to one line in this small terminals so dual line still need to be support. The actual dnf5 seems also to struggle with the output on small terminals. I did some testing and this looks mostly good expect some edge cases if the package name or version are to long then they could be truncated at the end.
|
Hello @j-mracek |
Where is |
is based from my testing in a virtual system normally you will see the new source. |
There has been a problem with transfer information from DNF4 but is is fixed now - #1403 |
The main thing I think it missing is highlighting vendor changes, which is the main detail missing for #750. |
Related issue: #326. |
The right-to-left format of the version column is odd to me. It's rather confusing for the version change to be formatted in that direction. Left-to-right would make much more sense and match the rest of the output. I'm not sure there is much value in having old and new values in the repository column. I think that one should just show the repository that the update is coming from. In most cases, that is going to be the same name written twice, i.e. |
The first column looks too wide for me - like it contained the section titles. How does it look with 80 cols terminal? |
I tested this a bit, and here are my thoughts:
In general, we should decide if there will also be a verbose version of the transaction table (e.g., when -v is passed). In that case, we can include all the info there. Otherwise, if there will only be one version of the output, I'd omit the repo and size changes. |
No, brackets don't change my mind on this. I think most people will view it in a linear way, left to right, old to new. Showing the new version first is going to lead to confusion. If the goal is to vertically align new versions, then I think the best approach is to have an old and new version column, using n/a as needed. This is what I originally proposed in #326. When we are restricted to an 80 col terminal, I think the easiest fix is to just suppress the architecture and size fields. Those are much less important than everything else. Here is an example of what that could look like for various types of transactions.
It even works with a multi-replace transaction.
|
I think in constrained rendering, we should just drop the old version info or maybe move it to a new line saying it replaces the old version. We also still need some indicator of vendor changes somehow, but that could probably be plugged into the same rendering logic for obsoletes. |
This is a complex topic, and not everyone will be satisfied. Our team discussed this and agreed to make minimal changes to the current default transaction table layout. For now, the primary concern mentioned repeatedly is fixing the "replacing" lines for identical package names. Additionally, we need to highlight vendor changes, as Neal mentioned. For a more verbose version, it would be better to enable it explicitly via a CLI or configuration option, rather than automatically based on window width. This approach will also simplify the fixes in our CI tests stack, where we parse the transaction table output. I've created a separate issue to follow up on the ideas presented here and continue the discussion. Thanks to everyone, especially @long-walk, for initiating and working on this sensitive topic. 🙂 |
Hello @jan-kolarik Only for reference the last state (code is not in state to push). So then I think we can close this PR and I will look in the progress of #1518 Thanks for your help. |
The transaction list should easy to review.
Patch Benefits: